Umutuzgur/sc 23256/pwt native code package simple #1042
Conversation
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
force-pushed
the
umutuzgur/sc-23256/pwt-native-code-package-simple
branch
from
20da986 to
c7899ae
Compare
WalkthroughThis change introduces comprehensive support for Playwright-based checks within the CLI tool. It adds new constructs, configuration schema extensions, Playwright project parsing, bundling, and uploading logic. The parser and project loader are refactored to handle Playwright configs and dependencies. New utilities, tests, and fixtures for Playwright are included, along with workflow improvements and dependency updates. Changes
Possibly related PRs
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
This comment has been minimized.
This comment has been minimized.
umutuzgur
added
build
ferrandiaz
added
build
This comment has been minimized.
This comment has been minimized.
ferrandiaz
added
build
This comment has been minimized.
This comment has been minimized.
ferrandiaz
added
build
|
🎉 Experimental release successfully published on npm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (15)
packages/cli/src/rest/checkly-storage.ts (1)
18-24: Use consistent header casing.The header naming convention is inconsistent: 'Content-Type' uses Pascal case while 'content-length' uses lowercase. HTTP headers are case-insensitive, but maintaining consistent casing improves code readability.
- { headers: { 'Content-Type': 'application/octet-stream', 'content-length': size } }, + { headers: { 'Content-Type': 'application/octet-stream', 'Content-Length': size } },packages/cli/src/reporters/list.ts (2)
48-48: Fix TODO comment - it references the wrong artifact type.The TODO comment refers to "print all video files URLs" but appears in the section handling trace links.
- // TODO: print all video files URLs + // TODO: print all trace files URLs
52-52: Fix TODO comment - it references the wrong artifact type.The TODO comment refers to "print all trace files URLs" but appears in the section handling video links.
- // TODO: print all trace files URLs + // TODO: print all video files URLspackages/cli/src/services/__tests__/playwright-config.spec.ts (1)
16-17: Fix duplicate test description and formatting.Both test cases use the same description, making it difficult to distinguish between them in test reports. Additionally, there's a missing space after the comma in the second test case.
- it('it should load simple config correctly', async () => { + it('should load config with explicit browsers correctly', async () => { const pwConfig = await loadFile(path.join(fixturesPath, 'simple-config.ts')) const config = new PlaywrightConfig(fixturesPath, pwConfig) expect(Array.from(config.testMatch)).toEqual(['**/*.@(spec|test).?(c|m)[jt]s?(x)']) expect(config.getBrowsers()).toEqual(['chromium', 'webkit', 'msedge', 'chrome']) }) -it('it should load simple config correctly', async () => { - const pwConfig = await loadFile(path.join(fixturesPath,'simple-config-no-browsers.ts')) +it('should load config with default browser fallback correctly', async () => { + const pwConfig = await loadFile(path.join(fixturesPath, 'simple-config-no-browsers.ts')) const config = new PlaywrightConfig(fixturesPath, pwConfig) expect(Array.from(config.testMatch)).toEqual(['tests.*.ts']) expect(config.getBrowsers()).toEqual(['chromium']) })packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts (1)
1-1: Remove unused import to keep the fixture minimal.
devicesis imported but never referenced because this config intentionally defines only a bare-bones Chromium project. Removing the unused symbol avoids future linter noise and keeps the fixture focused.-import { defineConfig, devices } from '@playwright/test'; +import { defineConfig } from '@playwright/test';packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts (1)
55-58: Hard-coded dev-server command might hang in CI.
npm run startusually starts a long-running dev server.
If the script does not exit after a successful compile Playwright will block.
Consider usingnpm run previewor a script that terminates once the server is ready, or add atimeoutkey.packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
6-7: Unused import produces lint noise.
pathToPosixis imported but never used in this spec file.-import { pathToPosix, loadFile } from '../../util' +import { loadFile } from '../../util'packages/cli/src/commands/test.ts (1)
237-248: New code to bundle Playwright checks during testing.This implementation is almost identical to the code in deploy.ts and could be refactored into a shared utility function.
Consider extracting this common bundling logic into a shared utility function to avoid duplication:
- for (const check of checks) { - // TODO: Improve bundling and uploading - if (!(check instanceof PlaywrightCheck) || check.codeBundlePath) { - continue - } - const { - relativePlaywrightConfigPath, browsers, key, - } = await PlaywrightCheck.bundleProject(check.playwrightConfigPath, check.include) - check.codeBundlePath = key - check.browsers = browsers - check.playwrightConfigPath = relativePlaywrightConfigPath - } + await bundlePlaywrightChecks(checks)And define a shared utility function in a common location:
// In a shared utility file export async function bundlePlaywrightChecks(checks: Check[]) { for (const check of checks) { // TODO: Improve bundling and uploading if (!(check instanceof PlaywrightCheck) || check.codeBundlePath) { continue } const { relativePlaywrightConfigPath, browsers, key, } = await PlaywrightCheck.bundleProject(check.playwrightConfigPath, check.include) check.codeBundlePath = key check.browsers = browsers check.playwrightConfigPath = relativePlaywrightConfigPath } }packages/cli/src/services/checkly-config-loader.ts (1)
170-174: Playwright config auto-detection is too narrow
findPlaywrightConfigPathonly checksplaywright.config.tsand.js.
Playwright officially supports.mts,.cts,.mjs,.cjsand plain.config.{js,ts}whenPW_CONFIG_FILEis set.Extending the list will avoid surprising “Unable to locate a config” errors for users on ESM/CTS modules.
packages/cli/src/services/util.ts (1)
297-304: Missingarchiveerror propagationOnly the
outputstream’s'error'event is handled. Ifarchiveritself emits
an error before piping data, the promise will hang forever.archive.pipe(output) +archive.on('error', reject)This guarantees the caller receives a rejected promise in all failure scenarios.
packages/cli/src/services/playwright-config.ts (1)
141-155: Default snapshot template may be duplicated across global & project scopesEvery project adds
DEFAULT_SNAPSHOT_TEMPLATEeven when the parent already has
it, causing redundant glob patterns later on.Consider adding only when the parent did not already include the default:
if (this.snapshotTemplates.size === 0 && !playwrightConfig.snapshotTemplates.has(DEFAULT_SNAPSHOT_TEMPLATE)) { this.snapshotTemplates.add(DEFAULT_SNAPSHOT_TEMPLATE) }packages/cli/src/services/check-parser/parser.ts (1)
220-255: Nit: duplicate regex evaluation on Windows path fallbackInside
createFileMatcher, every Windows path triggers a second loop overreListafter converting to file-URL. For largereListthis doubles the cost. You can avoid the extra loop whenreListis empty or by combining the checks:- if (path.sep === '\\') { + if (path.sep === '\\' && reList.length) {packages/cli/src/constructs/playwright-check.ts (3)
35-44: Duplicate state initialisation – favour base‐class handling
super(logicalId, props)already setsthis.logicalIdandthis.name; re-assigning them manually is unnecessary and can mask future changes in the base class.- this.logicalId = logicalId - this.name = props.name
87-99: Misleading variable name & cleanup of non-directory path
diris initialised as''and later holds the file path (outputFile), yet it is passed tocleanup, whose name implies it expects a directory.
Althoughfs.rmworks on files, this is confusing and could break if the implementation changes.- let dir = '' + let bundlePath = '' @@ - dir = outputFile - const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(dir) + bundlePath = outputFile + const { data: { key } } = await PlaywrightCheck.uploadPlaywrightProject(bundlePath) return { key, browsers, relativePlaywrightConfigPath } } finally { - await cleanup(dir) + await cleanup(bundlePath) }
81-85: Shell-injection risk in dynamically built test command
buildTestCommandconcatenates user-supplied project names and tags directly into the command string.
If any value contains spaces or shell metacharacters the invocation may break or allow injection.Consider constructing the command as an argv array and spawning via
child_process.spawn, or at minimumshlex.quoteeach interpolated value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts-snapshots/Google-test-1-Mobile-Chrome-linux.pngis excluded by!**/*.png
📒 Files selected for processing (22)
.github/workflows/release-canary.yml(2 hunks)packages/cli/package.json(2 hunks)packages/cli/src/commands/deploy.ts(3 hunks)packages/cli/src/commands/test.ts(4 hunks)packages/cli/src/constructs/index.ts(1 hunks)packages/cli/src/constructs/playwright-check.ts(1 hunks)packages/cli/src/reporters/list.ts(2 hunks)packages/cli/src/rest/checkly-storage.ts(1 hunks)packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts(1 hunks)packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts(1 hunks)packages/cli/src/services/__tests__/playwright-config.spec.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/tests/example.spec.ts(1 hunks)packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)packages/cli/src/services/check-parser/package-files/resolver.ts(0 hunks)packages/cli/src/services/check-parser/parser.ts(6 hunks)packages/cli/src/services/checkly-config-loader.ts(4 hunks)packages/cli/src/services/playwright-config.ts(1 hunks)packages/cli/src/services/project-parser.ts(4 hunks)packages/cli/src/services/util.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/cli/src/services/check-parser/package-files/resolver.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/cli/src/reporters/list.ts (1)
packages/cli/src/reporters/util.ts (1)
printLn(459-461)
packages/cli/src/commands/test.ts (1)
packages/cli/src/constructs/playwright-check.ts (1)
PlaywrightCheck(24-124)
packages/cli/src/commands/deploy.ts (1)
packages/cli/src/constructs/playwright-check.ts (1)
PlaywrightCheck(24-124)
packages/cli/src/constructs/playwright-check.ts (5)
packages/cli/src/constructs/check.ts (1)
CheckProps(17-108)packages/cli/src/constructs/validator-error.ts (1)
ValidationError(1-1)packages/cli/src/constructs/project.ts (1)
Session(146-221)packages/cli/src/services/util.ts (2)
bundlePlayWrightProject(274-305)cleanup(350-355)packages/cli/src/rest/api.ts (1)
checklyStorage(102-102)
🪛 GitHub Check: lint
packages/cli/src/commands/test.ts
[warning] 149-149:
'playwrightConfig' is assigned a value but never used
🔇 Additional comments (19)
packages/cli/package.json (1)
76-76:❓ Verification inconclusive
New dependencies added correctly for Playwright support
The addition of
@types/archiver,archiver, andminimatchaligns well with the need to bundle Playwright projects and handle file pattern matching. Using exact versions ensures reproducibility, though consider using the^prefix for patch-level security updates.Also applies to: 80-80, 94-94
🏁 Script executed:
#!/bin/bash # Check for any known vulnerabilities in the new dependencies echo "Checking for vulnerabilities in [email protected]" npm audit [email protected] --json || echo "No vulnerabilities found" echo "Checking for vulnerabilities in [email protected]" npm audit [email protected] --json || echo "No vulnerabilities found" echo "Checking npm registry info for latest versions" npm view archiver version npm view minimatch versionLength of output: 1765
Verify new dependencies and security in packages/cli/package.json (lines 76, 80, 94)
Please confirm the following before merging:
@types/[email protected]is the latest on npm.[email protected]is the current latest—no action needed.[email protected]is behind the latest10.0.1. Either:
• Upgrade to[email protected](verify compatibility)
• Or allow patch-level updates with a caret range (e.g.^9.0.5).- Generate a lockfile (
npm i --package-lock-only) and runnpm auditto catch any vulnerabilities in these packages.packages/cli/src/constructs/index.ts (1)
35-35: LGTM: Export correctly added for PlaywrightCheck constructThe new export follows the established pattern in the file and correctly makes the PlaywrightCheck construct available to consumers of the package.
packages/cli/src/reporters/list.ts (2)
45-53: LGTM! Improved formatting of individual links.The formatting changes for links in arrays improve readability by styling each link individually rather than styling the entire concatenated string. This creates a clearer visual indication of clickable elements.
79-87: LGTM! Consistently applied formatting improvement.The improved link formatting has been applied consistently across both methods that output links, maintaining a uniform user experience.
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts (1)
1-52: LGTM! Good test fixture for Playwright configuration parsing.This fixture provides a realistic Playwright configuration with various settings that would be common in real projects, making it a suitable test case for the Playwright integration.
packages/cli/src/services/__tests__/playwright-config.spec.ts (1)
9-22: LGTM! Good test coverage for Playwright configuration parsing.The tests verify two important scenarios: configs with explicit browsers and configs without browsers (testing default fallback). This ensures the parser works correctly in different situations.
packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts (1)
16-17:❌ Incorrect review comment
testMatchpattern is too restrictive and will not match nested specs.
'tests.*.ts'only matches files living in the repository root (e.g.tests.foo.ts).
For a typical directory structure (./tests/**/*.ts) no specs would be executed, which may lead to silent test runs.- testMatch: 'tests.*.ts', + // Run anything inside ./tests that ends with .spec.ts or .test.ts + testMatch: /.*\.(spec|test)\.ts/,
To verify how
testMatchis used elsewhere in our repo and ensure the suggestion aligns with our existing configs, let’s search for all occurrences oftestMatch:
🏁 Script executed:
#!/bin/bash # Find all usages of testMatch in the repo for context rg "testMatch" -nLength of output: 5802
🏁 Script executed:
#!/bin/bash sed -n '1,200p' packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.tsLength of output: 1397
🏁 Script executed:
#!/bin/bash sed -n '1,200p' packages/cli/src/services/__tests__/playwright-config.spec.tsLength of output: 1092
Ignore this suggestion—this is a test fixture, not a user‐facing default.
Thesimple-config-no-browsers.tsfile lives under__tests__/fixturesand intentionally uses a minimal'tests.*.ts'pattern to verify that the loader preserves whatever the user specifies. No changes are required here.Likely an incorrect or invalid review comment.
packages/cli/src/commands/deploy.ts (2)
14-14: Import list updated to include PlaywrightCheck construct.The import list now includes PlaywrightCheck which is needed for the Playwright integration.
117-119: Added Playwright configuration parameters to parseProject function.These parameters allow project parsing to detect and process Playwright test configurations properly.
.github/workflows/release-canary.yml (3)
22-31: Improved canary tag extraction using Node.js.The change replaces shell string manipulation with a more robust Node.js script that properly handles JSON parsing of PR labels.
40-43: Enhanced npm publish step with better logging.Added logging to show when publishing with an additional tag, making the workflow more transparent and easier to debug.
45-46: Explicitly defined environment variables for publishing step.Using explicit environment variables ensures the values are properly passed to the publishing command.
packages/cli/src/commands/test.ts (1)
186-188: Added Playwright configuration parameters to parseProject function.These parameters allow project parsing to detect and process Playwright test configurations properly.
packages/cli/src/services/project-parser.ts (5)
2-6: Refined imports using destructuring syntax.Changed simple imports to destructured imports for better organization of the utility functions.
12-15: Added imports for Playwright integration.These imports support the new Playwright functionality being added to the project parser.
32-34: Extended ProjectParseOpts interface with Playwright parameters.Added optional parameters to support Playwright project configuration in the project parser.
75-78: Normalized include parameter to always be an array.This ensures consistent handling of the include patterns regardless of how they were provided.
82-86: Improved performance with concurrent loading.Changed sequential loading of check types to concurrent loading using Promise.all, which will improve performance when parsing large projects.
packages/cli/src/services/check-parser/parser.ts (1)
182-183: Lost entry-point context inDependencyParseError
new DependencyParseError([].join(', '), …)now passes an empty string for the entrypoint parameter, removing valuable debugging context.Pass the offending file list or at least the current
filevariable instead:-throw new DependencyParseError([].join(', '), Array.from(missingFiles), [], []) +throw new DependencyParseError( + Array.from(resultFileSet).join(', '), + Array.from(missingFiles), + [], + [] +)
| test('Google test', async () => { | ||
| const browser = await chromium.launch(); | ||
| const context = await browser.newContext(); | ||
| const page = await context.newPage(); | ||
|
|
||
| // check start page is displayed | ||
| await page.goto('https://google.com'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test with assertions and proper resource cleanup
The test is missing assertions and browser cleanup, which could lead to resource leaks. Since this is a fixture, consider:
- Adding an assertion after navigation to validate page content
- Properly closing browser resources in a finally block
- Using Playwright's built-in fixtures to handle browser lifecycle
-import {test, chromium} from '@playwright/test';
+import {test, expect, chromium} from '@playwright/test';
test('Google test', async () => {
const browser = await chromium.launch();
const context = await browser.newContext();
const page = await context.newPage();
// check start page is displayed
await page.goto('https://google.com');
+ await expect(page).toHaveTitle(/Google/);
+
+ // Close resources
+ await context.close();
+ await browser.close();
});Alternatively, use Playwright's fixtures for cleaner resource management:
import {test, expect} from '@playwright/test';
test('Google test', async ({page}) => {
await page.goto('https://google.com');
await expect(page).toHaveTitle(/Google/);
});🤖 Prompt for AI Agents
In
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/tests/example.spec.ts
lines 3 to 10, the test lacks assertions and proper browser cleanup, risking
resource leaks. Refactor the test to use Playwright's built-in fixtures by
importing 'test' and 'expect' from '@playwright/test', then define the test
function to receive the 'page' fixture, navigate to 'https://google.com', and
add an assertion to check the page title matches /Google/. This approach
automatically manages browser lifecycle and ensures proper resource cleanup.
| test('Google test', async () => { | ||
| const browser = await chromium.launch(); | ||
| const context = await browser.newContext(); | ||
| const page = await context.newPage(); | ||
|
|
||
| // check start page is displayed | ||
| await page.goto('https://google.com'); | ||
| await expect(page).toHaveScreenshot() | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add resource cleanup to snapshot test
This test properly includes a visual snapshot assertion, which is good. However, it still lacks proper cleanup of browser resources. Additionally, consider:
- Adding a custom name to the screenshot for better maintainability
- Closing browser resources to prevent leaks
import {test, expect, chromium} from '@playwright/test';
test('Google test', async () => {
const browser = await chromium.launch();
const context = await browser.newContext();
const page = await context.newPage();
// check start page is displayed
await page.goto('https://google.com');
- await expect(page).toHaveScreenshot()
+ await expect(page).toHaveScreenshot('google-homepage.png');
+
+ // Close resources
+ await context.close();
+ await browser.close();
});Alternatively, use Playwright's fixtures for automatic resource management:
import {test, expect} from '@playwright/test';
test('Google test', async ({page}) => {
await page.goto('https://google.com');
await expect(page).toHaveScreenshot('google-homepage.png');
});🤖 Prompt for AI Agents
In
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts
around lines 3 to 11, the test launches browser resources but does not close
them, risking resource leaks. Fix this by either explicitly closing the page,
context, and browser after the test or refactor the test to use Playwright's
built-in fixtures by accepting the 'page' fixture as a parameter. Also, add a
custom name to the screenshot in the toHaveScreenshot call for clarity and
maintainability.
| projects: [ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 7'] }, | ||
| }, | ||
|
|
||
| /* Test against mobile viewports. */ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 5'] }, | ||
| }, | ||
| { | ||
| name: 'Mobile Safari', | ||
| use: { ...devices['iPhone 12'] }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Fix duplicate project names.
There are two projects named "Mobile Chrome" (lines 38 and 44). In a real configuration, this would cause ambiguity when selecting which project to run and might lead to unexpected behavior.
projects: [
{
name: 'Mobile Chrome',
use: { ...devices['Pixel 7'] },
},
/* Test against mobile viewports. */
{
- name: 'Mobile Chrome',
+ name: 'Mobile Chrome - Pixel 5',
use: { ...devices['Pixel 5'] },
},
{
name: 'Mobile Safari',
use: { ...devices['iPhone 12'] },
},
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projects: [ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 7'] }, | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| ], | |
| projects: [ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 7'] }, | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome - Pixel 5', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| ], |
🤖 Prompt for AI Agents
In
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts
between lines 36 and 51, there are two projects with the duplicate name "Mobile
Chrome." Rename one of these projects to a unique name to avoid ambiguity and
ensure each project can be distinctly identified when running tests.
| projects: [ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 7'] }, | ||
| }, | ||
|
|
||
| /* Test against mobile viewports. */ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 5'] }, | ||
| }, | ||
| { | ||
| name: 'Mobile Safari', | ||
| use: { ...devices['iPhone 12'] }, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate project names can shadow each other and break reporter output.
Both entries are called “Mobile Chrome”. Playwright keeps the last project with a duplicate name and silently discards the previous one, so the Pixel 7 run will never execute.
Give each project a unique name (or suffix with the device) to ensure all projects are honoured.
- {
- name: 'Mobile Chrome',
- use: { ...devices['Pixel 7'] },
- },
+ {
+ name: 'Mobile Chrome – Pixel 7',
+ use: { ...devices['Pixel 7'] },
+ },
...
- {
- name: 'Mobile Chrome',
- use: { ...devices['Pixel 5'] },
- },
+ {
+ name: 'Mobile Chrome – Pixel 5',
+ use: { ...devices['Pixel 5'] },
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projects: [ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 7'] }, | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| ], | |
| projects: [ | |
| { | |
| name: 'Mobile Chrome – Pixel 7', | |
| use: { ...devices['Pixel 7'] }, | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome – Pixel 5', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| ], |
🤖 Prompt for AI Agents
In
packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts
between lines 36 and 51, there are two projects named "Mobile Chrome," causing
the first to be ignored. Rename one of the "Mobile Chrome" projects to a unique
name, such as appending the device model (e.g., "Mobile Chrome Pixel 7" and
"Mobile Chrome Pixel 5"), to ensure both projects run and are reported
correctly.
| projects: [ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 7'] }, | ||
| snapshotDir: './snapshots-test', | ||
| snapshotPathTemplate: '{testDir}/snapshots-template/{arg}{ext}' | ||
| }, | ||
|
|
||
| /* Test against mobile viewports. */ | ||
| { | ||
| name: 'Mobile Chrome', | ||
| use: { ...devices['Pixel 5'] }, | ||
| }, | ||
| { | ||
| name: 'Mobile Safari', | ||
| use: { ...devices['iPhone 12'] }, | ||
| }, | ||
|
|
||
| /* Test against branded browsers. */ | ||
| { | ||
| name: 'Microsoft Edge', | ||
| use: { ...devices['Desktop Edge'], channel: 'msedge' }, | ||
| }, | ||
| { | ||
| name: 'Google Chrome', | ||
| use: { ...devices['Desktop Chrome'], channel: 'chrome' }, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure unique project names & verify Playwright version for snapshotDir / snapshotPathTemplate.
- The “Mobile Chrome” name collision (Pixel 7 vs Pixel 5) has the same shadowing risk mentioned earlier.
snapshotDir&snapshotPathTemplateare only available from Playwright 1.41+. If the toolchain installs an older version the config will throw at runtime.
- name: 'Mobile Chrome',
+ name: 'Mobile Chrome – Pixel 7',
...
- name: 'Mobile Chrome',
+ name: 'Mobile Chrome – Pixel 5',Add an engines/peer dependency constraint or pin Playwright ≥ 1.41 in package.json to avoid surprises.
🏁 Script executed:
#!/bin/bash
# Search for Playwright versions in package.json files across the repo
rg '"@playwright/test"\s*:\s*".+"' --context 1 --glob '*.json'
rg '"playwright"\s*:\s*".+"' --context 1 --glob '*.json'Length of output: 1217
Rename duplicate “Mobile Chrome” projects
The two “Mobile Chrome” entries will collide at runtime—please give each a unique name:
• In packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts
- Change the first project’s name to something like
Mobile Chrome – Pixel 7 - Change the second project’s name to something like
Mobile Chrome – Pixel 5
- {
- name: 'Mobile Chrome',
+ {
+ name: 'Mobile Chrome – Pixel 7',
use: { ...devices['Pixel 7'] },
snapshotDir: './snapshots-test',
snapshotPathTemplate: '{testDir}/snapshots-template/{arg}{ext}'
},
@@
- {
- name: 'Mobile Chrome',
+ {
+ name: 'Mobile Chrome – Pixel 5',
use: { ...devices['Pixel 5'] },
},Note: The repo already depends on Playwright 1.51.1 (≥1.41), so snapshotDir and snapshotPathTemplate are supported—no additional version pinning is needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projects: [ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 7'] }, | |
| snapshotDir: './snapshots-test', | |
| snapshotPathTemplate: '{testDir}/snapshots-template/{arg}{ext}' | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| /* Test against branded browsers. */ | |
| { | |
| name: 'Microsoft Edge', | |
| use: { ...devices['Desktop Edge'], channel: 'msedge' }, | |
| }, | |
| { | |
| name: 'Google Chrome', | |
| use: { ...devices['Desktop Chrome'], channel: 'chrome' }, | |
| }, | |
| projects: [ | |
| { | |
| name: 'Mobile Chrome – Pixel 7', | |
| use: { ...devices['Pixel 7'] }, | |
| snapshotDir: './snapshots-test', | |
| snapshotPathTemplate: '{testDir}/snapshots-template/{arg}{ext}' | |
| }, | |
| /* Test against mobile viewports. */ | |
| { | |
| name: 'Mobile Chrome – Pixel 5', | |
| use: { ...devices['Pixel 5'] }, | |
| }, | |
| { | |
| name: 'Mobile Safari', | |
| use: { ...devices['iPhone 12'] }, | |
| }, | |
| /* Test against branded browsers. */ | |
| { | |
| name: 'Microsoft Edge', | |
| use: { ...devices['Desktop Edge'], channel: 'msedge' }, | |
| }, | |
| { | |
| name: 'Google Chrome', | |
| use: { ...devices['Desktop Chrome'], channel: 'chrome' }, | |
| }, |
🤖 Prompt for AI Agents
In
packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts
between lines 36 and 62, there are two projects named "Mobile Chrome" which will
cause a name collision at runtime. Rename the first project to "Mobile Chrome –
Pixel 7" and the second project to "Mobile Chrome – Pixel 5" to ensure each
project has a unique name and avoid shadowing issues.
| this.projectName = '' | ||
| this.platform = 'linux' | ||
| this.testDir = playwrightConfig.testDir ? toAbsolutePath(dir, playwrightConfig.testDir) : dir | ||
| this.snapshotDir = playwrightConfig.snapshotDir ? toAbsolutePath(dir, playwrightConfig.snapshotDir) : this.testDir | ||
| this.files = new Set<string>() | ||
| this.snapshotTemplates = new Set<string>() | ||
| const testMatch = playwrightConfig.testMatch ?? ['**/*.@(spec|test).?(c|m)[jt]s?(x)'] | ||
| this.testMatch = new Set<string | RegExp>(Array.isArray(testMatch) ? testMatch : [testMatch]) |
There was a problem hiding this comment.
projectName is never initialised
PlaywrightConfig.projectName is left as an empty string; snapshot template
resolution later interpolates {projectName} and will produce paths like
/…/__snapshots__/directory//test.png.
If the root Playwright config has a name field, or the directory name should be
used, initialise it here.
-this.projectName = ''
+this.projectName = playwrightConfig.name ?? path.basename(dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.projectName = '' | |
| this.platform = 'linux' | |
| this.testDir = playwrightConfig.testDir ? toAbsolutePath(dir, playwrightConfig.testDir) : dir | |
| this.snapshotDir = playwrightConfig.snapshotDir ? toAbsolutePath(dir, playwrightConfig.snapshotDir) : this.testDir | |
| this.files = new Set<string>() | |
| this.snapshotTemplates = new Set<string>() | |
| const testMatch = playwrightConfig.testMatch ?? ['**/*.@(spec|test).?(c|m)[jt]s?(x)'] | |
| this.testMatch = new Set<string | RegExp>(Array.isArray(testMatch) ? testMatch : [testMatch]) | |
| this.projectName = playwrightConfig.name ?? path.basename(dir) | |
| this.platform = 'linux' | |
| this.testDir = playwrightConfig.testDir ? toAbsolutePath(dir, playwrightConfig.testDir) : dir | |
| this.snapshotDir = playwrightConfig.snapshotDir ? toAbsolutePath(dir, playwrightConfig.snapshotDir) : this.testDir | |
| this.files = new Set<string>() | |
| this.snapshotTemplates = new Set<string>() | |
| const testMatch = playwrightConfig.testMatch ?? ['**/*.@(spec|test).?(c|m)[jt]s?(x)'] | |
| this.testMatch = new Set<string | RegExp>(Array.isArray(testMatch) ? testMatch : [testMatch]) |
🤖 Prompt for AI Agents
In packages/cli/src/services/playwright-config.ts around lines 50 to 57, the
projectName property is initialized as an empty string but never set to a
meaningful value. To fix this, initialize projectName using the root Playwright
config's name field if available, or fallback to using the directory name. This
ensures that snapshot template paths correctly interpolate {projectName} and
avoid malformed paths.
| this.projectName = playwrightProject.name | ||
| this.platform = 'linux' | ||
| this.testDir = playwrightProject.testDir ? toAbsolutePath(dir, playwrightProject.testDir): playwrightConfig.testDir | ||
| this.snapshotDir = playwrightProject.snapshotDir ? toAbsolutePath(dir, playwrightProject.snapshotDir): playwrightProject.testDir | ||
| this.testDir = playwrightProject.testDir ?? playwrightConfig.testDir | ||
| this.snapshotDir = playwrightProject.snapshotDir ?? this.testDir | ||
| this.files = new Set<string>() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate & conflicting assignment of testDir / snapshotDir
PlaywrightProject sets testDir / snapshotDir twice – first with absolute
paths, then immediately with potentially relative values, undoing the previous
normalisation.
- this.testDir = playwrightProject.testDir ?? playwrightConfig.testDir
- this.snapshotDir = playwrightProject.snapshotDir ?? this.testDirRemove the second assignment to keep the correctly resolved absolute paths.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.projectName = playwrightProject.name | |
| this.platform = 'linux' | |
| this.testDir = playwrightProject.testDir ? toAbsolutePath(dir, playwrightProject.testDir): playwrightConfig.testDir | |
| this.snapshotDir = playwrightProject.snapshotDir ? toAbsolutePath(dir, playwrightProject.snapshotDir): playwrightProject.testDir | |
| this.testDir = playwrightProject.testDir ?? playwrightConfig.testDir | |
| this.snapshotDir = playwrightProject.snapshotDir ?? this.testDir | |
| this.files = new Set<string>() | |
| this.projectName = playwrightProject.name | |
| this.platform = 'linux' | |
| this.testDir = playwrightProject.testDir | |
| ? toAbsolutePath(dir, playwrightProject.testDir) | |
| : playwrightConfig.testDir | |
| this.snapshotDir = playwrightProject.snapshotDir | |
| ? toAbsolutePath(dir, playwrightProject.snapshotDir) | |
| : playwrightProject.testDir | |
| this.files = new Set<string>() |
🤖 Prompt for AI Agents
In packages/cli/src/services/playwright-config.ts around lines 130 to 136, the
properties testDir and snapshotDir are assigned twice: first with absolute paths
using toAbsolutePath, then immediately reassigned with potentially relative
values, which overrides the normalized paths. Remove the second assignments to
testDir and snapshotDir to retain the correctly resolved absolute paths and
avoid conflicting values.
| const projects = playwrightConfig.projects ?? [playwrightConfig] | ||
| for (const project of projects) { | ||
| // Cache the files by test dir | ||
| const files = await this.collectFiles(cachedFiles, project.testDir, ignoredFiles) | ||
| const matcher = this.createFileMatcher(Array.from(project.testMatch)) | ||
| for (const file of files) { | ||
| if (!matcher(file)) { |
There was a problem hiding this comment.
Potential runtime crash when testMatch is undefined
Array.from(project.testMatch) assumes that project.testMatch is always iterable.
However, Playwright allows this property to be omitted – in which case it is undefined, triggering TypeError: Array.from requires an array-like or iterable object.
Guard against the absence of testMatch, e.g.:
-const matcher = this.createFileMatcher(Array.from(project.testMatch))
+const matcher = this.createFileMatcher(
+ project.testMatch ? Array.from(project.testMatch) : ['**/*.{spec,test}.{js,ts,mjs}']
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const projects = playwrightConfig.projects ?? [playwrightConfig] | |
| for (const project of projects) { | |
| // Cache the files by test dir | |
| const files = await this.collectFiles(cachedFiles, project.testDir, ignoredFiles) | |
| const matcher = this.createFileMatcher(Array.from(project.testMatch)) | |
| for (const file of files) { | |
| if (!matcher(file)) { | |
| // Cache the files by test dir | |
| const files = await this.collectFiles(cachedFiles, project.testDir, ignoredFiles) | |
| const matcher = this.createFileMatcher( | |
| project.testMatch | |
| ? Array.from(project.testMatch) | |
| : ['**/*.{spec,test}.{js,ts,mjs}'] | |
| ) | |
| for (const file of files) { | |
| if (!matcher(file)) { | |
| // … |
🤖 Prompt for AI Agents
In packages/cli/src/services/check-parser/parser.ts around lines 200 to 206, the
code assumes project.testMatch is always iterable by calling
Array.from(project.testMatch), but testMatch can be undefined causing a runtime
TypeError. Fix this by adding a guard to check if project.testMatch exists and
is iterable before calling Array.from, or provide a default empty array when it
is undefined to safely create the matcher.
| private async getFilesFromPaths (playwrightConfig: (PlaywrightConfig)): Promise<string[]> { | ||
| const ignoredFiles = ['**/node_modules/**', '.git/**'] | ||
| const cachedFiles = new Map<string, string[]> | ||
| // If projects is definited, ignore root settings |
There was a problem hiding this comment.
Syntax error – missing constructor parentheses on Map instantiation
new Map<string, string[]> lacks the trailing () and will therefore be parsed as a type-only expression, producing TS1005: ';' expected at compile time.
-const cachedFiles = new Map<string, string[]>
+const cachedFiles: Map<string, string[]> = new Map()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async getFilesFromPaths (playwrightConfig: (PlaywrightConfig)): Promise<string[]> { | |
| const ignoredFiles = ['**/node_modules/**', '.git/**'] | |
| const cachedFiles = new Map<string, string[]> | |
| // If projects is definited, ignore root settings | |
| private async getFilesFromPaths (playwrightConfig: PlaywrightConfig): Promise<string[]> { | |
| const ignoredFiles = ['**/node_modules/**', '.git/**'] | |
| const cachedFiles: Map<string, string[]> = new Map() | |
| // If projects is definited, ignore root settings | |
| … | |
| } |
🤖 Prompt for AI Agents
In packages/cli/src/services/check-parser/parser.ts around lines 196 to 199, the
instantiation of the Map object is missing parentheses. Fix the syntax error by
adding empty parentheses after Map<string, string[]> to correctly instantiate
the Map, changing `new Map<string, string[]>` to `new Map<string, string[]>()`.
| export interface PlaywrightCheckProps extends CheckProps { | ||
| playwrightConfigPath: string | ||
| codeBundlePath?: string | ||
| installCommand?: string | ||
| testCommand?: string | ||
| pwProjects?: string|string[] | ||
| pwTags?: string|string[] | ||
| browsers?: string[] | ||
| include?: string|string[] | ||
| groupName?: string | ||
| logicalId: string | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant logicalId property in props causes API misuse
PlaywrightCheckProps requires a logicalId, yet the constructor already receives logicalId as its first positional argument and ignores the value in props.
This forces callers to supply the same value twice and risks accidental divergence.
Recommend dropping the field from the interface:
export interface PlaywrightCheckProps extends CheckProps {
playwrightConfigPath: string
codeBundlePath?: string
@@
- groupName?: string
- logicalId: string
+ groupName?: string
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface PlaywrightCheckProps extends CheckProps { | |
| playwrightConfigPath: string | |
| codeBundlePath?: string | |
| installCommand?: string | |
| testCommand?: string | |
| pwProjects?: string|string[] | |
| pwTags?: string|string[] | |
| browsers?: string[] | |
| include?: string|string[] | |
| groupName?: string | |
| logicalId: string | |
| } | |
| export interface PlaywrightCheckProps extends CheckProps { | |
| playwrightConfigPath: string | |
| codeBundlePath?: string | |
| installCommand?: string | |
| testCommand?: string | |
| pwProjects?: string|string[] | |
| pwTags?: string|string[] | |
| browsers?: string[] | |
| include?: string|string[] | |
| groupName?: string | |
| } |
🤖 Prompt for AI Agents
In packages/cli/src/constructs/playwright-check.ts between lines 11 and 23,
remove the redundant logicalId property from the PlaywrightCheckProps interface
since logicalId is already passed as the first positional argument to the
constructor and the value in props is ignored. This prevents API misuse by
avoiding the need to supply logicalId twice and eliminates the risk of
inconsistent values.
I hereby confirm that I followed the code guidelines found at engineering guidelines
Affected Components
Notes for the Reviewer
New Dependency Submission